Skip to content

Health-check heuristic fix, operator drain/block controls, and websocket close-code correctness - #526

Merged
oten91 merged 22 commits into
mainfrom
fix/cometbft-health-check-heuristic
Aug 7, 2026
Merged

oten91 merged 22 commits into
mainfrom
fix/cometbft-health-check-heuristic

Conversation

@oten91

@oten91 oten91 commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Twenty-two commits that accumulated on this branch. Grouped by area below.

Health checks

  • Thread the JSON-RPC method into both heuristic sites. The health-check payload never set JSONRPCMethod, so the heuristic saw the method as "/" and the CometBFT health carve-out never fired — 16 cosmos services failed the health check 100% of the time. A uniform penalty distorts no ranking and triggers no alert, which is why it went unnoticed.
  • Catch the config schema up to the health-check keys that already ship.

Operator controls

  • POST /admin/reputation/drain/{serviceId} — temporarily bench every scored endpoint of one operator for a service, to answer "where would this traffic go if operator X were unavailable" without waiting for X to fail. Fleet-wide via shared storage, hard expiry ceiling, reported on its own gauge so a deliberate bench never reads as a quality incident.
  • blocked_domains (gateway config, PATH_BLOCKED_DOMAINS to widen at restart) — permanently excludes every endpoint at a domain from given RPC types across all services. Where a drain is temporary, per-service, and yields rather than empty a pool, this does not yield. Covers session selection, fallback endpoints, and health checks; runs before the supplier allowlist so Target-Suppliers cannot override it. A malformed entry refuses to boot rather than silently narrowing the ban.

Six follow-up commits fix defects found while validating the drain in production, all the same shape — reported success while excluding nothing. Root cause each time: the test asserted on a value other than the one the production caller receives. selection_harness_test.go closes that gap by asserting through the real selection path, including across session rotation and with a preferred endpoint supplied.

WebSocket

  • Close health-check probes, rebinds, and SIGTERM teardown with a proper handshake instead of dropping the socket. Operators were logging a continuous 1006 flood that traced to our probe, not to their infrastructure.
  • Send each peer a close code that fits its role.
  • Per-connection frame-rate histogram. A per-domain sum cannot distinguish one firehose plus a hundred idle sockets from a hundred ordinary subscribers; those have opposite explanations.

Docs

Corrected two claims in CLAUDE.md that had gone stale, and replaced real operator domains in test fixtures and examples with neutral placeholders.

Testing

go build ./... clean; unit tests pass. reputation/storage testcontainer tests require Docker and were not run locally.

oten91 and others added 22 commits August 6, 2026 23:03
A CometBFT `health` check answers {"jsonrpc":"2.0","id":1,"result":{}} when the
node is healthy. The heuristic's empty-object rule flagged exactly that, at
confidence 0.95, on every healthy node every cycle.

The carve-out for it already existed in analyzeJSONRPC: skip the rule when the
rpc type is COMET_BFT or isCometBFTMethod(method) holds. Neither guard could
fire. Cosmos services declare the check as type json_rpc, and the method never
reached the heuristic at all.

Two separate sites dropped it, and fixing either one alone changes nothing
observable:

  1. buildServicePayload never set JSONRPCMethod. User traffic gets that field
     at QoS parsing time; a health check does not go through QoS, so it stayed
     empty and protocol/shannon/context.go fell back to payload.Path — "/" for
     a JSON-RPC check, matching no method.

  2. The executor runs its own heuristic.Analyze after the relay returns, and
     that call passed a hardcoded "". This is the site that decides the check's
     outcome, and it runs only when site 1 passes.

So repairing site 1 by itself just hands the response to site 2, which rejects
it on the same rule. The check stays at 100% failure and only the metric label
moves — from status_code="error" to status_code="200", both major_error.

Both sites now resolve the method the way the other four callers do (hedge.go,
http_request_context_handle_request.go x3): payload method, Path as fallback,
so REST path-aware rules are unaffected.

Measured 2026-08-06 against production: 16 cosmos services failing this check
100% of the time, one passing. That single check accounted for ~19% of all
failing health-check signals fleet-wide.

The penalty was uniform across every endpoint on those services, so it distorted
no ranking and tripped no alert — it only depressed the absolute baseline.
Scores on cosmos services will rise once this ships, and any threshold tuned
while it was in effect was tuned against that depressed baseline.

Tests drive ExecuteCheckViaProtocol through a mock protocol rather than calling
heuristic.Analyze directly: a test that calls the analyzer restates its rules and
passes with this fix reverted. The EVM case is pinned too — an empty-object
result for eth_blockNumber must still be rejected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Several keys the loader has read for a while were missing from the schema, and
active_health_checks sets additionalProperties: false — so a valid config that
used them was flagged as invalid in the editor.

Adds backend_dedup, sync_allowance (global and per-service), max_workers, and
the per-check enabled / headers / archival / sync_check. Corrects the check
`type` description, which still named the pre-rename "jsonrpc" and carried no
enum, and adds fatal_error to reputation_signal.

Schema only. Nothing here changes how a config is parsed: the loader uses a
non-strict yaml.Unmarshal, so these keys already worked at runtime and unknown
ones are still ignored.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds POST /admin/reputation/drain/{serviceId}, which writes a cooldown
expiry onto every scored endpoint of one operator (eTLD+1) for a service.
Selection already excludes endpoints in cooldown regardless of score, so
this reuses a filter every selection path is guaranteed to consult rather
than adding a second exclusion mechanism that some path could miss.

Motivation: "where would this traffic go if operator X were unavailable"
was only answerable by waiting for X to fail. Tumbling a websocket
connection is not a substitute -- a tumble re-dials but leaves every
operator eligible, so the connection can land straight back where it
started. Measured on gnosis: 8 consecutive tumbles failed to move a
~500 frames/s subscription off the two operators already carrying it,
because each rebind could reselect them.

This is NOT a penalty. Value, CriticalStrikes and RecentCriticalRate are
left untouched, so the quality signal stays readable while a drain is in
effect -- which matters, because reading it is usually the entire point of
draining. A drain that rewrote the score would destroy the measurement it
exists to enable.

rpc_type narrows the drain to one protocol, so benching an operator's
websocket endpoints leaves its json_rpc and rest traffic in place.

Release (duration=0) lifts only cooldowns this drain wrote and that nothing
has overwritten since. A cooldown earned for real while the drain was up
survives -- otherwise "undo my experiment" would silently un-bench a
legitimately failing endpoint, the one outcome nobody would intend.

Known limit: the cooldown lives on the score, so an endpoint the reputation
service has never observed is treated by selection as "initial score, not
in cooldown" and stays selectable through a drain. The response carries
unscored_warning rather than letting a partial drain read as airtight.

Per-pod in-memory state like the other admin endpoints, so it must be
issued to each pod. Expires on its own and does not survive a restart.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds path_websocket_connection_frame_rate: every live connection's
endpoint→client frames/sec, observed on each sampler pass, bucketed by
service_id and domain.

Why: path_websocket_messages_total is a per-domain SUM, and a sum cannot
distinguish "one firehose plus a hundred idle sockets" from "a hundred
ordinary subscribers". Those have opposite explanations -- the first is
where a high-volume client happened to land, the second is a property of
the operator -- and the distinction is not academic. Measured fleetwide:
one operator holds 16.9% of websocket connections and earns 66.1% of
websocket relays, a 3.9x over-index that the existing sums cannot
attribute. On gnosis, 2 connections out of ~70 carried ~97% of frames, so
a handful of connections can produce that entire fleetwide number without
the operator doing anything. Only the distribution separates the two.

Both frame directions are reward-eligible under the relay miner (each
endpoint→client push is signed and mined paired with the most recent
request), so this is closer to a settlement-volume distribution than a
traffic one.

Emitted from the existing sampleRates pass rather than a new ticker, so the
histogram and the ranking a capped tumble is spent on can never disagree --
otherwise a dashboard could justify a tumble the tumble itself would then
decline to make.

Idle connections are observed at 0 deliberately. Dropping them would leave
the quantiles describing only the connections that carry traffic, which is
exactly the population the metric exists to be measured against.

Observations are collected under the registry lock but recorded after
releasing it: a Prometheus histogram takes its own internal lock, and
nesting that inside the registry mutex would put an unrelated subsystem on
the critical path of every admin tumble and rebind.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Documents POST /admin/reputation/drain/{serviceId} alongside the other
admin endpoints, and path_websocket_connection_frame_rate.

Also records the finding that motivated both: every endpoint→client
WebSocket frame is signed and mined as a reward-eligible relay, so one
eth_subscribe anchors an unbounded number of payable relays and the push
rate is chosen by the supplier being paid. A per-domain frames/s number is
therefore a settlement-volume meter, not a load meter -- the opposite of
how the existing panel description read it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects in the drain endpoint, both found by using it against mainnet.

1. The bench did not survive a storage refresh.

The drain wrote CooldownUntil onto the Score. refreshFromStorage overwrites
the local cache from Redis unconditionally, so every drain evaporated on the
next refresh tick -- while the endpoint still reported drained=N. Observed
live: bsc endpoints showed 0 in cooldown minutes after a successful drain,
and traffic returned to the drained operator on its own.

The bench now lives in drainedKeys and is applied as an overlay when scores
are read, so nothing that rewrites scores can erase it. Releasing also
becomes exact rather than best-effort: because the drain never touches the
Score, a cooldown the endpoint earned on its own cannot be disturbed by a
release, by construction rather than by an equality check.

Same trap CLAUDE.md already documents for the circuit breaker's
refreshFromRedis, walked into from the other direction.

2. It could only be targeted by node id.

Key granularity is per-service config, so the same operator is a hostname on
one service and a pokt1... supplier address on another. An eTLD+1 filter
returned matched=0 on supplier-keyed services while looking successful. The
handler now resolves an eTLD+1, hostname, or full URL against live endpoint
details into every identifier a key could carry, and reports
identifiers_resolved / matched_endpoints so a failed resolution is
distinguishable from a resolution that matched nothing.

Resolution lives in the router because only the protocol layer holds the
supplier->URL mapping needed to bridge granularities.

Testing: the previous tests asserted Score.CooldownUntil was set, which is
exactly the field that got clobbered -- they passed while the mechanism was
broken. Tests now assert through GetScores/IsInCooldown, which is the call
selection actually makes (FilterByScore ignores cooldown entirely). Verified
the regression tests have teeth by neutering the overlay: 5 of 9 fail.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A drain was pod-local, so benching an operator meant hitting all N pods.
In practice that produced partial drains nobody realised were partial --
observed while trying to bench two operators across 16 replicas.

Drains now live in shared storage under a dedicated __drains__ hash and
every replica adopts them on its next refresh, so one admin call applies
fleet-wide. A hash rather than one key per drain: the refresh reads the
whole set each tick, so it is one HGETALL instead of a SCAN, and the score
keyspace SCAN cannot pick it up.

Deliberately NOT stored on the Score. That is what the previous fix was
about: refreshFromStorage overwrites the cache from storage unconditionally,
so anything carried on Score.CooldownUntil is erased within a refresh cycle.
Drains and scores stay strictly separate.

Refresh REPLACES the local set rather than merging, which is what makes a
release propagate; merging would leave a released drain benched forever on
whichever replica did not issue it. A storage failure leaves the local set
intact instead of clearing it -- losing Redis mid-incident must not silently
un-bench everyone -- and a failed write reports propagation_error with a
THIS POD ONLY warning rather than implying a fleet-wide bench.

Expiry, so a drain can be set and forgotten:
  - duration capped at 2h, rejected rather than clamped (silently shortening
    a drain an operator believed they set is worse than telling them)
  - TTL on the shared key, extended never shortened (ExpireGT), so a short
    drain cannot cut short a longer one in the same hash
  - expired fields filtered on read and reaped, since Redis cannot TTL
    individual hash fields
There is no way to bench an operator indefinitely through this endpoint.

Testing: 14 drain tests, including two replicas over shared storage proving
a drain and a release each reach a pod that never received an admin call,
that an expired drain is never applied, that a storage outage keeps existing
drains, and that a failed write is reported as pod-local.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verifies the drain overlay cannot leak into anything that scores or
persists reputation:

- the raw cached Score keeps CooldownUntil zero while benched, and nothing
  benched reaches storage. A leak here would leave an operator carrying a
  real cooldown after a 20-minute experiment, making the reputation record
  a lie about their behaviour.
- the rate-cooldown escalation ladder does not see it. That ladder keys off
  the PREVIOUS CooldownUntil and benches longer on each consecutive trip,
  so a visible drain would silently escalate an operator's next real
  cooldown -- punishing them for something we did.
- recovering the score neither lifts the drain nor picks up the overlay.

Isolation holds because RecordSignal and recoverScore read/build the raw
Score, and the overlay is applied only on the GetScore/GetScores read paths.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A drain removes an operator's traffic exactly the way a cooldown does, so
path_endpoints_in_cooldown counted both -- making a deliberate bench
indistinguishable from a quality incident on every dashboard.

That is backwards for this tool in particular: a drain is usually applied
in order to OBSERVE an operator, so publishing it as a cooldown corrupts
the signal the drain exists to produce, and would eventually have someone
paged over a bench we applied ourselves.

path_endpoints_in_cooldown now counts only cooldowns the endpoint EARNED,
reading the un-overlaid score via the new GetScoreRaw. Drains are published
on their own gauge, path_endpoints_drained{domain, rpc_type, service_id},
sourced from IsDrained. Selection still excludes both -- only reporting
distinguishes them.

The two counters share one enumeration helper differing solely in the
include predicate, so they can never drift in which endpoints they walk.

Also adds a test pinning the split: a drained endpoint and an endpoint with
a genuinely earned cooldown must each appear in exactly one of the two
metrics, while both stay excluded from selection.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The drain did not work in production for more than one session, and
reported success the whole time.

DrainDomain resolved the target to a fixed set of EndpointKeys and benched
those. EndpointAddr is `supplierAddr-url`, and a session rotates its
supplier set every rollover (~20 min), so the benched keys went stale while
the live endpoints at that operator had never been benched. Selection
picked them freely. path_endpoints_drained kept publishing the stale count,
so it looked like it was holding.

Observed on gnosis: path_endpoints_drained reported 60 and 160 endpoints
benched on mainnet while /ready on the same pod showed 0 in cooldown for
every operator, and every tumbled connection rebound straight back onto a
"drained" operator (rpcgate->spacebelt, spacebelt->rpcgate, never the third
operator). None of that was evidence about the operators; the drain simply
was not in force.

Drains are now stored as a PREDICATE -- (service, domain, rpc_type) ->
expiry -- and evaluated at selection time against the endpoint's LIVE URL,
in the shannon reputation filter. An endpoint rotated into the session at a
drained operator is benched the moment it appears, and an unscored endpoint
is benched too, since matching no longer depends on a score existing.

This also removes the score overlay entirely: GetScore is raw again, so
GetScoreRaw and IsDrained are gone, replaced by IsDomainDrained. The drain
still never touches a Score, so the quality signal stays readable while an
operator is benched, and path_endpoints_in_cooldown still reports only
earned cooldowns.

Empty rpc_type covers every protocol for the service. A drain can never
empty the endpoint pool -- if it would, the drained endpoints are restored
and it logs, because a drain is an operator preference, not a correctness
constraint, and an outage is not a redistribution. Hostname and URL targets
widen to the operator, since benching a single machine would be defeated by
the next rotation.

Testing: the previous tests all drained against a static cache inside one
test body, so nothing rotated the keys and none of them could fail. The
regression test now asserts the bench holds across a rotated supplier set,
which is the property that actually broke.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ban was completely inert in production while reporting success.

filterByReputation builds its result by walking `cached`, but the drain
block deleted from the `endpoints` map passed in -- which nothing reads
after `cached` is built. So no endpoint was ever excluded, the Warn log
never fired (the counter stayed 0), and both the API response and
path_endpoints_drained reported the operator benched.

Observed on eth: both brands banned in both environments, 2 connections
carrying 16.5 f/s tumbled off rpcgate, and the traffic came straight back to
rpcgate. Same on gnosis, where every tumbled connection rebound onto a
"banned" operator. None of that was evidence about the operators.

The drain now filters `cached` itself, so every downstream path -- including
the pool-collapse guard -- operates on the surviving set.

Testing: this is the third bug in this feature that shipped because the test
asserted on a helper rather than on the value the caller receives. The new
test drives filterByReputation end to end and asserts on the RETURNED map;
verified it fails without the fix. Also pins that a drain is scoped to its
rpc_type, and that banning every operator yields rather than emptying the
pool.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three bugs shipped in the admin drain, all with passing tests, all the same
mistake: asserting on something the author wrote rather than on the value the
production caller receives. Score field that a storage refresh erased, key
snapshot that session rotation invalidated, and a map the filter did not
build its result from. Each reported success in production while excluding
nothing.

In all three the real question was identical -- does selection still return
this endpoint -- and there was no cheap way to ask it, so each fix grew unit
tests around the edges instead.

selectionScenario makes it one call:

    s := newSelectionScenario(t, "gnosis", spacebeltA, rpcgateA, kaloriusA)
    s.Drain("spacebelt.xyz", RPCType_WEBSOCKET, time.Hour)
    s.AssertExcluded(RPCType_WEBSOCKET, spacebeltA)
    s.RotateSuppliers(1)
    s.AssertExcluded(RPCType_WEBSOCKET, spacebeltA)

RotateSuppliers is the important part: it re-fronts each backend URL with new
supplier addresses while leaving the URLs alone, which is what a session
rollover does. Anything keyed on EndpointAddr silently goes stale there, and
the key-snapshot drain passed every test until this was expressible.
harnessEndpoint keeps supplier and URL separate for that reason; mockEndpoint
collapses them into one field, which is part of why the bug was invisible.

Six tests covering all three shipped bugs plus rpc_type scoping, release, the
never-empty-the-pool guard, and the independence of an earned cooldown from an
admin bench. Verified 5 of 6 fail against bug 3 by reverting the fix.

Also adds the three rules to CLAUDE.md: assert on the caller's return value,
revert-and-confirm-red before committing a fix, and treat two observables
disagreeing about one state as a bug rather than picking the convenient one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rebind path dropped the endpoint TCP socket without a WebSocket close
handshake. To the endpoint that is a peer vanishing mid-read, and rebinds
fire on EVERY session rollover, so this filled operator logs continuously
across the whole fleet.

Confirmed independently by two operators running different clients:

  geth        websocket: bad close code 1006 ... api=websocket-server
  Nethermind  An exception caused the WebSocket to enter the Aborted state
              (Failed: 0)

Both were reported as suspected node problems. Neither was; it is PATH.

Normal shutdown already sends a close frame (bridge.go) -- only the rebind
path skipped it, which is why the noise looked constant rather than
occasional. Note #521 fixed 1006 in the CLIENT direction; the endpoint
direction was never covered.

Sends 1000 Normal Closure: a rebind is an orderly move, not a fault on the
endpoint's part, and a 1006 misattributes our routing decision as their
error. Best effort with a 1s deadline -- a rollover often begins because the
endpoint closed on us, and a rebind must not stall being polite to a socket
that is already gone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The admin-drain work added IsDomainDrained/DrainDomain to ReputationService but
only updated the mocks in the packages whose tests were run at the time, so
qos/evm has been failing to compile since. Stubs only: drains are asserted
through protocol/shannon's selection harness, which checks what selection
actually returns rather than what a mock was told.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S993uPDkQAbhpEAdMupmAP
…ket drop

Node operators reported a continuous stream of `websocket: close 1006
(abnormal closure): unexpected EOF` (geth) and `An exception caused the
WebSocket to enter the Aborted state (Failed: 0)` (Nethermind), all tagged
connection_source=gateway. The previous fix addressed the session-rebind path,
which was a real instance of the same bug but the wrong site by two orders of
magnitude: rebinds run at 0.6/s fleetwide while the websocket health-check
probe dials, measures and closes at ~100/s. Every one of those probes ended in
a bare conn.Close(), which sends no close frame, so the endpoint's read fails
mid-frame and it reports the peer as having vanished. The probe was added
recently, which is why the noise appeared to start from nothing.

These are our teardowns being logged as the endpoint's fault, and they are
indistinguishable in an operator's logs from a real client disconnect — so the
signal an operator would use to find an actual problem was buried under ~8.9M
synthetic abnormal closures a day.

CloseEndpointConn centralises the handshake and is best-effort by construction:
the write is unchecked and deadline-bounded, because a teardown often begins
BECAUSE the peer already went away and a rebind must not stall being polite to
a socket that is not there. Applied to the probe, to the two subscription-replay
failure paths that dropped a freshly dialled connection, and the rebind path now
routes through it rather than repeating the sequence inline.

1000 Normal Closure throughout: none of these are faults on the endpoint's part,
and telling it otherwise misattributes our own routing decisions as its errors.

The test asserts on what the ENDPOINT observes, via a real websocket server —
asserting that a close helper was called would prove nothing, since the defect
was precisely that a bare Close() produces a read error rather than a close
frame on the peer. Reverting the fix reproduces the reported string exactly.

Known gap, not addressed here: http.Server.Shutdown does not close hijacked
connections, so a rollout still drops every live websocket without a handshake.
That is one burst per deploy rather than a continuous rate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S993uPDkQAbhpEAdMupmAP
… them

http.Server.Shutdown explicitly does not close hijacked connections, and every
websocket is hijacked, so nothing in the termination path touched them: the
process exited, every socket died with its TCP connection, and both peers saw
an abnormal closure. The client could not tell a deploy from a crash, and the
endpoint logged a fault it did not cause — one 1006 burst per rollout, every
service and application address inside the same second.

The sweep snapshots the registry's controllers and releases the lock BEFORE
closing anything. bridge.Close runs shutdown(), which deregisters, which takes
the same mutex for writing; closing while holding even the read lock
self-deadlocks. Tumble gets away with holding it only because it is a
non-blocking channel send. The test that covers this deregisters from inside
Close exactly as production does — a fake that only records the call cannot
observe the deadlock at all, and reverting to the naive version hangs it.

Closes run concurrently under a five-second ceiling. Serially, at a one-second
write deadline per frame, a busy replica would overrun its 30s termination
grace period and get SIGKILLed, producing precisely the abrupt teardown this
is meant to prevent. The returned count is what completed rather than what was
attempted, so a sweep that timed out half way cannot read as a clean one.

Clients get 1012 Service Restart, which exists for this and tells them to come
back — onto a replica that is not terminating. Endpoints get an orderly close
rather than a truncated read.

Behind an interface assertion rather than a method on gateway.Protocol: holding
live bridges is a property of the implementation, and a protocol that holds
none needs nothing here.

Also fixes a pre-existing data race in the idle-reaper tests, where the bridge
goroutine outlived the test and kept reading the package vars that cleanup
restored — it failed -race for the whole package, which is coverage these
changes should not ship without.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S993uPDkQAbhpEAdMupmAP
shutdown() computed one close code and wrote the same frame to both peers. That
is right when propagating (the endpoint's 4000 at session expiry should reach
the client verbatim) and wrong for anything PATH initiates, because PATH is a
different role to each side:

  external client <--(PATH is the server)-- PATH --(PATH is the client)--> relay miner

RFC 6455 defines 1011/1012/1013 as things a SERVER tells a client. Sent
upstream they invert the roles: "service restarting, please reconnect"
addressed to the relay miner asks it to reconnect to us, which is not something
it does, and "internal server error" reports our own fault as though the
endpoint had one. The gateway-shutdown path added in the previous commit would
have sent exactly that.

The endpoint now gets 1001 Going Away, which RFC 6455 defines for both
directions and which describes what happened: the peer that dialed you is
leaving. The client still gets 1012, which is correct and useful facing that
way — it tells the client to come back, onto a replica that is not terminating.
Codes meaning the same thing in both directions, and application codes
(3000-4999), pass through untouched.

Not a protocol error either way — gorilla accepts 1012 on read. This is about
the operator on the other end being told something true.

The test reads the close frame off BOTH ends of one live bridge, since the
defect is precisely that the directions were not distinguished; either side
examined alone passed before the fix.

Also waits for the bridge goroutine to exit, like the idle tests: it reads
package vars other tests restore, and leaving it running races them under -race.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S993uPDkQAbhpEAdMupmAP
…ound to

The drain exempted requestedEndpointAddr, and every websocket path supplies
one. ReconnectEndpoint passes preferredAddr — the endpoint the connection is
ALREADY bound to — so the exemption fired on exactly the connections a drain
exists to move. A websocket connection re-selects at every session rollover,
and each of those rollovers re-picked the drained endpoint because it was
"preferred".

Measured in production: drain applied fleet-wide, path_endpoints_drained
reporting 170 endpoints benched, every connection tumbled, and four minutes
later the drained operator still served 73% of the service's websocket frames.
Sticky placement is the thing a drain overrides; it cannot also be the thing
that protects an endpoint from one.

The pool-empty branch is the real safety net and is sufficient on its own — an
endpoint is kept only when dropping it would leave nothing to serve. Covered by
its own test rather than assumed.

Why three passing tests missed this: the harness's Survivors() hardcodes "" for
requestedEndpointAddr, which is the one call shape the websocket path never
uses. SurvivorsPreferring closes that gap, and the new test fails against the
old code with the production symptom.

This is the fourth defect in this feature with an identical shape — reports
success, excludes nothing — and the fourth where the test asserted on something
other than the value the production caller receives.

Two things that made the diagnosis slower and are worth recording: drain
propagation was never the cause (storage refresh is 5s, sixty seconds before
the tumble), and grepping pod logs for the drain's Warn diagnostics returned
zero on every pod because LOG_LEVEL=error suppresses them — the instrument, not
the signal. This feature's only diagnostics are invisible in production.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S993uPDkQAbhpEAdMupmAP
…y service

blocked_domains (gateway_config) permanently excludes every endpoint at a
domain — eTLD+1 or exact hostname — from the given RPC types on ALL services.
PATH_BLOCKED_DOMAINS appends entries at pod-restart speed (union: env can
widen a ban, never narrow one). A malformed entry refuses to boot instead of
silently narrowing the ban.

Where a drain is temporary, per-service, and yields rather than empty the
pool, this is the nuclear option, and every property follows from that:

- Covers every path that hands out endpoints: session selection (which feeds
  HTTP, WebSocket bind/rebind, retry/hedge/batch), fallback endpoints (which
  bypass every session-endpoint filter and needed explicit coverage), and
  health checks (paid relays — the ban stops the probes too).
- Runs before the supplier allowlist, so Target-Suppliers cannot override it
  — the header does bypass drains, which is a documented gap.
- No preferred-endpoint exemption, and matching is on the live URL, so it
  holds across session rollovers by construction (drain bugs 4 and 2).
- It can empty the pool: failing the request beats serving it from an
  operator the config says must never serve it.

path_blocked_domains_configured reports the loaded entries at any LOG_LEVEL;
path_endpoints_domain_blocked_total counts actual removals per selection
pass — configured-but-never-engaging is the lying-instrument signature that
shipped four drain bugs, so the two must be read together.

Tests assert through the production callers (getSessionsUniqueEndpoints,
getUniqueEndpoints, GetEndpointsForHealthCheck), and each of the three call
sites was revert-checked: filter removed, tests fail, filter restored.

Known limit: a ban covering only a subset of the HTTP-carried types leaves
the endpoint's other HTTP health probes running (the executor keys HTTP
checks on endpoint address, not per-type URL). All-type and websocket bans
are exact.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every blocklist test hand-built Protocol{blockedDomains: ...}, leaving the
constructor wiring — env merge, compile, field assignment — with zero
coverage: deleting the assignment kept all tests green while shipping an
inert ban, the exact shape of all four drain bugs.

Two tests drive the real constructor: one proves a ban present only in
PATH_BLOCKED_DOMAINS excludes through selection on the constructed
instance (the field being set is necessary but not sufficient), one proves
a malformed env entry refuses to boot. Revert-checked: deleting the
assignment fails the first test and only it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The drain's "not airtight — unscored endpoints stay selectable" limitation no
longer exists. The predicate rewrite moved the check onto the endpoint's live URL
and ahead of GetScores, so an endpoint carrying no score is excluded like any
other. Kept the history so the score-based bench is not reintroduced, and
replaced the limitation with the constraint that is actually load-bearing: a
drain is rollover-paced. Endpoints leave the selectable set at once, but a bound
websocket connection only moves at its own next rebind. Measured in production
today: 42 connections cleared in ~19 min and 26 in ~11 min, on rollover alone
with no tumble.

path_supplier_exhausted_total is no longer 0 fleetwide. It fires on exactly one
service, bursting 0 to ~11/s across ~4h with long zero troughs between. The
earlier reading was an instant query that landed in a trough — the same mistake
recurred while checking it today, so the file now says to read it over a range.
The conclusion it supported is unchanged: on every other service the brake is
dormant.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S993uPDkQAbhpEAdMupmAP
Test fixtures, code comments and configuration examples named real operator
domains. This repository is public, so those names were about to become a
permanent record of which providers were blocked and from what.

Replaced throughout with neutral placeholders (op-alpha.example ... and matching
Go identifiers). Nothing about the assertions depends on the strings being real
domains: the blocklist matches on eTLD+1 or exact hostname, and the placeholders
exercise both, including the mixed-case path that proves case-insensitive
resolution.

No behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S993uPDkQAbhpEAdMupmAP
@oten91
oten91 merged commit d5ef007 into main Aug 7, 2026
11 of 12 checks passed
@oten91
oten91 deleted the fix/cometbft-health-check-heuristic branch August 7, 2026 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant